Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for OpenAPI v3.1 #55

Closed

Conversation

JamesHinshelwood
Copy link
Contributor

Resolves #50 .

I'm leaving this as a draft for now so people can take a look at the approach and ideally try using this branch to see if it meets their needs. I'll be doing the same for a couple of weeks as I'm not yet convinced that this is the right way of adding 3.1 support.

Also note that lots of the lines changed in this PR were due to moving the 3.0 support to a separate sub-module. It would probably make sense to review the commits separately.

JamesHinshelwood and others added 6 commits February 12, 2022 10:15
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
@ahl
Copy link
Collaborator

ahl commented Mar 19, 2022

I was skeptical of this approach, but seeing it in action, I'm warming to it. I haven't been through it in fine details, but I had a couple of high level points and questions.

  • I think a desirable outcome would be if this were compatible with past versions by default. I could see this as excessively limiting, but it would be nice if there were a way to introduce this compatibly into version 1.x of this crate, get some use, and then make an incompatible change with 2.0. For example, in lib.rs do a pub use v3_0::* and rename the multi-version enum to something like OpenAPIVersion or OpenAPIGeneric (neither of those names is great). Alternatively, I could imagine just telling people to s/openapiv3::/openapiv3::v3_0::/g and that would suffice...

  • What would you think of a "upgrade" function that would convert 3.0.x to 3.1? It seems pretty straightforward (https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0). For consumers of OpenAPI documents, putting into a canonical form seems very helpful.

  • One of the key benefits of OpenAPI v3.1 is convergence with JSON Schema. I really like that you're using schemars::schema::Schema here! I'm not sure, however, about flattening the untagged enum. I also think while we don't need to add extensions which are part of SchemaObject we will need to add example and discriminator (https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#fixed-fields-20).

  • With regard to this review, I appreciate the wrapping of comments, but perhaps we could pull that out to minimize the diffs?

  • There's a lot of code/comment in comment between v3_0 and v3_1. It might be worth considering how we share that code where it's literally identical e.g. via include! (since use self::* could replace use crate::v3_X::*).

In summary: this is great stuff; let's move it forward.

@ahl
Copy link
Collaborator

ahl commented Mar 19, 2022

Also: you might want to run this through the repo I've used for testing: https://github.com/ahl/openapiv3-test

/// object, the behavior is undefined. See the rules for resolving Relative
/// References.
#[serde(rename = "$ref", skip_serializing_if = "Option::is_none")]
reference: Option<String>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this field public, so we continue to be able to use struct literal syntax for these!

/// object, the behavior is undefined. See the rules for resolving Relative
/// References.
#[serde(rename = "$ref", skip_serializing_if = "Option::is_none")]
reference: Option<String>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this field public, so we continue to be able to use struct literal syntax for these!

Comment on lines +20 to +23
/// Inline extensions to this object. Additional properties MAY omit the
/// `x-` prefix within this object.
#[serde(flatten)]
pub extensions: IndexMap<String, serde_json::Value>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be removed; schemars::schema::Schema already includes extensions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlowe - Can you clarify that? I don't see any mention of extensions in schemars::schema::Schema.
Meanwhile, it appears that the x-* extension convetion is still part of the 3.1 schema.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SchemaObject has a serde-flattened extensions field here:
https://github.com/GREsau/schemars/blob/master/schemars/src/schema.rs#L165

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I've been unable to find this in the spec: Does jsonschema or schemars use the x-* prefix?
If not, we should probably still support such functionality.

Copy link

@rrichardson rrichardson May 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind. I see that the functionality of the two is pretty much identical. So my last comment didn't make any sense. I agree it should probably go away.

@rrichardson
Copy link

@JamesHinshelwood - Are you still available to carry this to victory? If not, I'd be happy to clone your branch and make the necessary changes.

@ahl
Copy link
Collaborator

ahl commented May 21, 2022

@rrichardson I think at this point it would be reasonable to build on this work if you have cycles. Please see my comments above if you're going to take a swing #55 (comment)

@rrichardson
Copy link

@ahl - I will work on this today.

One question regarding Security Schemes: In The SecuritySchemes Spec they casually mention

This object MAY be extended with Specification Extensions.

How do you propose we deal with this? (I think it's true for 3.0 as well)
This is of special interest to me, because my company uses openAPI to generate Protobuf, among other things, so we take advantage of the fact that we're not specifically tied to the limited set of Auth mechanisms that are "supported" by OpenAPI

@rrichardson
Copy link

One more thing: Regarding upgrades, it seems the most idiomatic approach would be just to impl TryFrom<v3_0::Foo> for v3_1::Foo (From, I'm not sure if they're perfectly compatible) for every type, so that anyone upgrading could just

let my_old_schema: v3_0::OpenAPI = serde_json::frrom_str(&x).unwrap(); 
let my_new_schema: v3_1::OpenAPI = my_old_schema.try_into().unwrap(); 
let bytes = serde_json::to_vec(my_new_schema).unwrap();

I guess the question is: Where should we put such an impl? Part of me says that v3_1 and v3_0 shouldn't know about each other. The other part says that it should be acceptable if newer versions know about older versions.

Thoughts?

@dlowe
Copy link

dlowe commented May 23, 2022

I'm ambivalent about including conversion logic in this library (which in my mind is just serde (de)serialization implementations!), but if you do:

The other part says that it should be acceptable if newer versions know about older versions.

^ I agree with this :)

@ahl
Copy link
Collaborator

ahl commented May 23, 2022

How do you propose we deal with this?

I think we need to add extensions to each of the SecurityScheme variants; alternatives?

Agreed on the conversion approach.

One more wrinkle for 3.1: the schemars crate seems to be only sporadically maintained. I think it's fine to prototype against it, but before we publish a version that depends upon it, I think we should get clarity on its future... or fork the repo if it's a dead end.

@rrichardson
Copy link

One more wrinkle for 3.1: the schemars crate seems to be only sporadically maintained.

It seems to get a new version every couple of months, it was updated as recently as 6 days ago.
The PRs and Issues, while numerous, do seem to make some progress.

It does seem like it could use a couple more reviewers/committers, but it seems actively maintained.

I guess we'll see in a bit how it performs in the context of OpenAPI.

@rrichardson rrichardson mentioned this pull request May 24, 2022
@rrichardson
Copy link

Ok. I't not quite ready, but it is close. I need to do something about the From<> for Schema.
#58

@JamesHinshelwood
Copy link
Contributor Author

Sorry I dropped this, thanks for picking it up @rrichardson - Your PR looks good to me FWIW. Since this PR is a subset of yours, I will close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for OpenAPI v3.1.x
4 participants